Replace parameter 'master_timeout' with 'cluster_manager_tiemout' in RequestConverters of High-Level-Rest-Client #2683

tlfeng · 2022-03-31T05:43:18Z

Description

This PR can only be merged after all the other PRs listed in the description of issue #2511 (except #2744) are merged.
[^ Prerequisite achieved]

In server directory, the REST API parameter master_timeout is deprecated in version 3.0 (and will be removed in 4.0) to support inclusive naming, and alternative parameter cluster_manager_timeout is introduced (in version 2.0, in issue #2511).
Correspondingly, the new parameter should be used in the High-Level-Rest-Client, which will be done by this PR.

Please note the change will break the compatibility for High-Level-Rest-Client version 3.0 with server 1.x (and below), which means High-Level-Rest-Client version 3.0 will only support server 2.x and 3.x (4.x not exist yet).

Use parameter cluster_manager_timeout instead of master_timeout in High-Level-Rest-Client RequestConverters class for building REST requests (Otherwise :client:rest-high-level:asyncIntegTest tests will be broken caused by using master_timeout generating deprecation warning) (in commit 2dfb976)
Modify corresponding unit tests
Change lots of "master timeout" in internal method and class names. (in commit a2bcb0c)

Issues Resolved

A part of issue #2511

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Tianli Feng <[email protected]>

opensearch-ci-bot · 2022-03-31T06:12:28Z

❌ Gradle Check failure d15b979dd227f527d665ef823145908ec479eaae
Log 3967

Reports 3967

opensearch-ci-bot · 2022-03-31T06:18:19Z

❌ Gradle Check failure 2dfb976
Log 3968

Reports 3968

Signed-off-by: Tianli Feng <[email protected]>

opensearch-ci-bot · 2022-04-13T19:06:00Z

✅ Gradle Check success cda5e04
Log 4447

Reports 4447

…out(TimeValue) in RequestConverters Signed-off-by: Tianli Feng <[email protected]>

opensearch-ci-bot · 2022-04-13T23:17:28Z

✅ Gradle Check success 0962c11
Log 4454

Reports 4454

Signed-off-by: Tianli Feng <[email protected]>

tlfeng · 2022-04-13T23:21:03Z

client/rest-high-level/src/main/java/org/opensearch/client/RequestConverters.java

        Params withMasterTimeout(TimeValue masterTimeout) {
            return putParam("master_timeout", masterTimeout);
        }

+        Params withClusterManagerTimeout(TimeValue clusterManagerTimeout) {
+            return putParam("cluster_manager_timeout", clusterManagerTimeout);
+        }


Note: this is the core change for this PR, using parameter "cluster_manager_timeout" instead of "master_timeout" to build REST requests can avoid the deprecation warning.

opensearch-ci-bot · 2022-04-13T23:48:21Z

✅ Gradle Check success a2bcb0c
Log 4455

Reports 4455

kotwanikunal · 2022-04-14T00:12:49Z

Looking at the files, it looks like a direct replacement of the terms, even in the case of tests even though the parameter is just deprecated. Should we add in duplicate tests around the deprecated term to ensure that the change is non breaking? We can test for the warning instead.

tlfeng · 2022-04-14T00:20:47Z

Looking at the files, it looks like a direct replacement of the terms, even in the case of tests even though the parameter is just deprecated. Should we add in duplicate tests around the deprecated term to ensure that the change is non breaking?

Hi @kotwanikunal Thanks for reviewing!
Hmm, actually it's not necessary to deprecate the old method Params withMasterTimeout(TimeValue masterTimeout) 😂, because the whole RequestConverter class is an internal class (which isn't exposed to the public, seeing from the class access modifier)
So I don't think adding test for the deprecated "master_timeout" parameter in the REST Client is necessary, because nobody from outside can directly call the method. Do you think it's better for me to remove the old method Params withMasterTimeout(TimeValue masterTimeout) instead of deprecation?

tlfeng · 2022-04-14T01:08:38Z

@kotwanikunal Thanks for the reminding. I didn't realize that this will be a breaking change for the High Level Rest Client, which makes it can not communicate with OpenSearch 1.x.

The problem is:
Currently the High Level Rest Client will add the optional parameter master_timeout for every 60 API that accepts this parameter when building the REST API requests.
Keep using master_timeout will cause client test failures due to a deprecation warning in HTTP response header.
Change to use cluster_manager_timeout will break the compatibility for the client with server version 1.x .

There are 3 options to deal with the parameter in High Level Rest Client:

Break the compatibility with OpenSearch 1.x sever to build REST API requests with new parameter cluster_manager_timeout in High-Level-Rest-Client version 2.x. (In the meantime, I think 2.x server has broken the compatibility with 1.x client due to the mapping type removal)
Use deprecated parameter master_timeout to build REST API requests, make 2.x client keep the compatibility with 1.x server. This will result a deprecation warning in the response header for the 60 APIs.
Create an additional method for each of the 60 APIs to allow build REST API requests with either old or new parameter. This seems requires lots of code change. It will make 2.x client keep the compatibility with 1.x server.

In the documentation, it's said 1.x client works with 1.x server, so it's reasonable for the 2.x client to only support 2.x server.
What do you think?

tlfeng · 2022-04-14T21:38:44Z

A follow up, I will find out the exact impact to users about the deprecation warning in the response header. If the impact is subtle, I will keep using "master_timeout" in the RestClient to build requests and add "expectWarnings()" into tests.

tlfeng · 2022-04-15T09:43:45Z

Replaced by PR #2912 to keep the backwards compatibility of Rest Client 2.x with server 1.x .

tlfeng · 2022-04-15T21:00:04Z

The change can exist in version >= 3.0

opensearch-ci-bot · 2022-04-15T21:32:46Z

✅ Gradle Check success a2bcb0c
Log 4527

Reports 4527

tlfeng added enhancement Enhancement or improvement to existing feature or request v2.0.0 Version 2.0.0 backport 2.x Backport to 2.x branch backport 2.0 Backport to 2.0 branch blocked Identifies issues that are blocked labels Mar 31, 2022

tlfeng mentioned this pull request Mar 31, 2022

Add a new request parameter "cluster_manager_timeout" in the REST APIs that accept "master_timeout" parameter, and deprecate "master_timeout" #2511

Closed

11 tasks

Use cluster_manager_tiemout in rhlc

2dfb976

Signed-off-by: Tianli Feng <[email protected]>

tlfeng force-pushed the rhlc-cluster-manager-timeout-simple branch from d15b979 to 2dfb976 Compare March 31, 2022 05:49

tlfeng changed the title ~~Replace parameter 'master_timeout' with 'cluster_manager_tiemout' in RequestConverters of Rest-High-Level-Client~~ Replace parameter 'master_timeout' with 'cluster_manager_tiemout' in RequestConverters of High-Level-Rest-Client Mar 31, 2022

Merge branch 'main' into rhlc-cluster-manager-timeout-simple

cda5e04

Signed-off-by: Tianli Feng <[email protected]>

tlfeng marked this pull request as ready for review April 13, 2022 18:31

tlfeng requested a review from a team as a code owner April 13, 2022 18:31

tlfeng removed the blocked Identifies issues that are blocked label Apr 13, 2022

Deprecate withMasterTimeout(TimeValue) and add withClusterManagerTime…

0962c11

…out(TimeValue) in RequestConverters Signed-off-by: Tianli Feng <[email protected]>

Rename setRandomMasterTimeout() to setRandomClusterManagerTiemout()

a2bcb0c

Signed-off-by: Tianli Feng <[email protected]>

tlfeng commented Apr 13, 2022

View reviewed changes

tlfeng added the >breaking Identifies a breaking change. label Apr 14, 2022

tlfeng closed this Apr 15, 2022

tlfeng added v3.0.0 Issues and PRs related to version 3.0.0 and removed v2.0.0 Version 2.0.0 backport 2.x Backport to 2.x branch backport 2.0 Backport to 2.0 branch labels Apr 15, 2022

tlfeng reopened this Apr 15, 2022

tlfeng mentioned this pull request Apr 16, 2022

Replace REST API request parameter "master_timeout" with "cluster_manager_timeout" in High Level REST Client when building requests #2928

Closed

4 tasks

dblock approved these changes Apr 19, 2022

View reviewed changes

tlfeng merged commit 18f4495 into opensearch-project:main Apr 19, 2022

tlfeng deleted the rhlc-cluster-manager-timeout-simple branch April 19, 2022 18:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace parameter 'master_timeout' with 'cluster_manager_tiemout' in RequestConverters of High-Level-Rest-Client #2683

Replace parameter 'master_timeout' with 'cluster_manager_tiemout' in RequestConverters of High-Level-Rest-Client #2683

tlfeng commented Mar 31, 2022 •

edited

Loading

opensearch-ci-bot commented Mar 31, 2022

opensearch-ci-bot commented Mar 31, 2022

opensearch-ci-bot commented Apr 13, 2022

opensearch-ci-bot commented Apr 13, 2022

tlfeng Apr 13, 2022

opensearch-ci-bot commented Apr 13, 2022

kotwanikunal commented Apr 14, 2022 •

edited

Loading

tlfeng commented Apr 14, 2022 •

edited

Loading

tlfeng commented Apr 14, 2022 •

edited

Loading

tlfeng commented Apr 14, 2022

tlfeng commented Apr 15, 2022

tlfeng commented Apr 15, 2022

opensearch-ci-bot commented Apr 15, 2022

Replace parameter 'master_timeout' with 'cluster_manager_tiemout' in RequestConverters of High-Level-Rest-Client #2683

Replace parameter 'master_timeout' with 'cluster_manager_tiemout' in RequestConverters of High-Level-Rest-Client #2683

Conversation

tlfeng commented Mar 31, 2022 • edited Loading

Description

Issues Resolved

Check List

opensearch-ci-bot commented Mar 31, 2022

opensearch-ci-bot commented Mar 31, 2022

opensearch-ci-bot commented Apr 13, 2022

opensearch-ci-bot commented Apr 13, 2022

tlfeng Apr 13, 2022

Choose a reason for hiding this comment

opensearch-ci-bot commented Apr 13, 2022

kotwanikunal commented Apr 14, 2022 • edited Loading

tlfeng commented Apr 14, 2022 • edited Loading

tlfeng commented Apr 14, 2022 • edited Loading

tlfeng commented Apr 14, 2022

tlfeng commented Apr 15, 2022

tlfeng commented Apr 15, 2022

opensearch-ci-bot commented Apr 15, 2022

tlfeng commented Mar 31, 2022 •

edited

Loading

kotwanikunal commented Apr 14, 2022 •

edited

Loading

tlfeng commented Apr 14, 2022 •

edited

Loading

tlfeng commented Apr 14, 2022 •

edited

Loading